Extensible Fixed Statement #24469

VSadov · 2018-01-26T06:51:07Z

Implemented "exstensible fixed statements"

The feature expands support of fixed to types that provide a special ref-returning method as described in https://github.com/dotnet/csharplang/blob/master/proposals/csharp-7.3/pattern-based-fixed.md
(currently only arrays and strings can be used directly in fixed)

NOTE: the name of the method is likely to change. It will be updated in subsequent changes.

VSadov · 2018-02-09T19:26:07Z

@dotnet/roslyn-compiler - please review. This is a 7.3 feature PR to a feature branch.

jcouv · 2018-02-12T18:37:00Z

src/Compilers/CSharp/Portable/BoundTree/GenerateNodes.bat

 REM Copyright (c)  Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.

 REM Run the node generator to create new bound tree node definitions.
-REM You must run this in the directory containing Generated.cs.


I think we should remove this file.
I've been using build\scripts\generate-compiler-code.cmd. That one updates all the generated files at once. #Closed

jcouv · 2018-02-12T18:38:56Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+            return true;
+        }
+
+        private MethodSymbol GetDangerousGetPinnableReferenceMethod(BoundExpression initializer, DiagnosticBag additionalDiagnostics)


Nit: no need for "Dangerous" here #WontFix

I think we are not keeping "Dangerous" in the name. Once I implement the name change this will be renamed too

In reply to: 167647561 [](ancestors = 167647561)

jcouv · 2018-02-12T18:42:45Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

-                            elementType = ((BoundAddressOfOperator)initializerOpt).Operand.Type;
+                        getPinnableMethod = GetDangerousGetPinnableReferenceMethod(initializerOpt, additionalDiagnostics);
+
+                        bool extensisbleFixedEnabled = ((CSharpParseOptions)initializerOpt.SyntaxTree.Options)?.IsFeatureEnabled(MessageID.IDS_FeatureExtensibleFixedStatement) != false;


typo in "extensible" #Closed

jcouv · 2018-02-12T18:43:37Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                        // check for String
+                        // NOTE: We will allow DangerousGetPinnableReferenceMethod to take precendence, but only if it is a member of System.String
+                        if (initializerType.SpecialType == SpecialType.System_String &&
+                            ((object)getPinnableMethod == null || !extensisbleFixedEnabled || getPinnableMethod.ContainingType.SpecialType != SpecialType.System_String))


extensisbleFixedEnabled [](start = 67, length = 23)

I don't think the presence/absence of the feature flag should affect binding. Only diagnostics should be conditional on the feature flag.
We could probably remove extensisbleFixedEnabled entirely and rely on CheckFeatureAvailability below. #Closed

This is a very special case. We want that extensible pinnable to win over when implemented on System.String.

In reply to: 167648804 [](ancestors = 167648804)

You a right though - we should not check extensisbleFixedEnabled here.
The implementors of System.String will guarantee backwards compatibility of the new pattern. So this is more a matter of runtime versioning, not compiler versioning.

In reply to: 169196056 [](ancestors = 169196056,167648804)

jcouv · 2018-02-12T18:45:11Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                        if (extensisbleFixedEnabled)
+                        {
+                            // not a specially known type, check for getPinnableMethod
+                            if (getPinnableMethod != null)


getPinnableMethod [](start = 32, length = 17)

cast to object? #Closed

jcouv · 2018-02-12T18:45:42Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                                break;
                            }
-                            else
+                            else if (additionalDiagnostics != null)


Seems like additionalDiagnostics is always set. #Closed

not always. it is reported only on cases when we found the member, but something was wrong with it.
The idea is to help implementers of the method, but not complain about the method when it is clearly not provided.

In reply to: 167649336 [](ancestors = 167649336)

jcouv · 2018-02-12T18:46:50Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

-                    // convert to generate any additional conversion errors
-                    initializerOpt = GenerateConversionForAssignment(declType, initializerOpt, diagnostics);
-                    return true;
+                    // did not find anythig callable


typo: anything #Closed

jcouv · 2018-02-12T18:48:09Z

src/Compilers/CSharp/Test/Emit/CodeGen/UnsafeTests.cs

+                );
+        }
+
+        #endregion Custom fixed statement tests


#endregion Custom fixed statement tests [](start = 7, length = 40)

Can we test the following?

fixed (int* p = (dynamic)new Fixable())

GetPinnable that returns ref readonly

check semantic model (although it looks like the selected GetPinnable method will not be exposed)

test with ambiguous GetPinnable method (maybe one instance method and one extension, or maybe two extensions) #Closed

Fixed is currently not exposed in semantic model at all, so there is no way to test the differences.
For other suggestions more tests were added.

In reply to: 167650001 [](ancestors = 167650001)

jcouv · 2018-02-12T18:49:36Z

src/Compilers/CSharp/Portable/CSharpResources.resx

+    <value>The given expression cannot be used in a fixed statement</value>
+  </data>
+  <data name="ERR_FixableHelperDoesNotFitThePattern" xml:space="preserve">
+    <value>The method '{0}' does not meet requirements necessary </value>


The method '{0}' does not meet requirements necessary [](start = 11, length = 53)

I feel both messages should give more information about what is expected. #Closed

Logged an issue to follow up with more detailed diagnostics - #24947

In reply to: 167650429 [](ancestors = 167650429)

jcouv · 2018-02-12T18:50:24Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

-                else
+
+                var analyzedArguments = AnalyzedArguments.GetInstance();
+                BoundExpression getPinnableReferenceCall = BindMethodGroupInvocation(initializer.Syntax, initializer.Syntax, methodName, (BoundMethodGroup)boundAccess, analyzedArguments, bindingDiagnostics, queryClause: null, allowUnexpandedForm: false);


Nit: please break those extra-long lines #Closed

jcouv · 2018-02-12T18:53:13Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+            DiagnosticBag bindingDiagnostics = DiagnosticBag.GetInstance();
+            try
+            {
+                var boundAccess = BindInstanceMemberAccess(initializer.Syntax, initializer.Syntax, initializer, methodName, rightArity:0, typeArgumentsSyntax: default, typeArguments: default, invoked:true, bindingDiagnostics);


invoked:true [](start = 192, length = 12)

Nit: formatting on invoked:true and some other arguments #Closed

jcouv · 2018-02-12T18:56:30Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs


-            return true;
+                var getPinnableReferenceMethod = call.Method;
+                if (getPinnableReferenceMethod is ErrorMethodSymbol ||


Can we avoid testing a specific type? #Closed

This is how we test for error method symbol in other places.

In reply to: 167652407 [](ancestors = 167652407)

jcouv · 2018-02-12T19:02:24Z

src/Compilers/CSharp/Test/Emit/CodeGen/UnsafeTests.cs

+                // (6,25): error CS8320: Feature 'extensible fixed statement' is not available in C# 7.2. Please use language version 7.3 or greater.
+                //         fixed (int* p = new Fixable())
+                Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion7_2, "new Fixable()").WithArguments("extensible fixed statement", "7.3").WithLocation(6, 25),
+                // (6,25): error CS9365: The given expression cannot be used in a fixed statement


CS9365 [](start = 33, length = 6)

I think the only error in this code is the LangVer related one. #Closed

jcouv · 2018-02-12T19:05:13Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

-            if (ReferenceEquals(initializerOpt, null))
+            if (initializerOpt?.HasAnyErrors != false)
            {
                return false;


Does a test cover this path? #Closed

Added a test

In reply to: 167654777 [](ancestors = 167654777)

jcouv · 2018-02-12T19:06:08Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs


        // Note: you will need to re-generate compiler code after adding warnings (build\scripts\generate-compiler-code.cmd)
+
+        ERR_ExprCannotBeFixed = 9365,


Please insert the error codes above the comment #Closed

jcouv · 2018-02-12T19:07:39Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_FixedStatement.cs

+        /// fixed(int* ptr = &amp;v){ ... }    == becomes ===>
+        /// 
+        /// pinned ref int pinnedTemp = ref v;    // pinning managed ref
+        /// int* ptr = (int*)&amp;pinnedTemp;         // unsafe cast to unmanaged ptr


Nit: use CDATA to avoid escaping ampersand #Resolved

With the CDATA, "&" can be changed to "&" on the first line.

In reply to: 167655416 [](ancestors = 167655416)

jcouv · 2018-02-12T19:22:07Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_FixedStatement.cs

+
+            // .GetPinnable()
+            var getPinnableCall = getPinnableMethod.IsStatic?
+                                            factory.Call(null, getPinnableMethod, callReceiver):


Nit: different indentation than the rest of the code #Closed

jcouv · 2018-02-12T19:24:27Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_FixedStatement.cs

+            }
+
+            // .GetPinnable()
+            var getPinnableCall = getPinnableMethod.IsStatic?


The meaning here is really to distinguish regular methods from extension methods, is that right? #Closed

Yes, but we are already in lowering, so we should emit an ordinary static call.

In reply to: 167659923 [](ancestors = 167659923)

jcouv · 2018-02-12T20:11:35Z

src/Compilers/CSharp/Test/Emit/CodeGen/UnsafeTests.cs

+    }
+";
+
+            var compVerifier = CompileAndVerify(text, options: TestOptions.UnsafeReleaseExe, verify: Verification.Fails, expectedOutput: @"5456");


verify: Verification.Fails [](start = 93, length = 26)

Would this be verifiable by some updated verification rules? #Closed

The ref-returning method will be, but any use of int * will not

In reply to: 167672486 [](ancestors = 167672486)

jcouv · 2018-02-12T20:24:52Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

-                        Error(diagnostics, ErrorCode.ERR_ManagedAddr, initializerSyntax, elementType);
-                        hasErrors = true;
+                        // See ExpressionBinder::BindPtrToArray (though most of that functionality is now in LocalRewriter).
+                        var arrayType = (ArrayTypeSymbol)initializerType;


Consider inlining, since used once #Closed

jcouv · 2018-02-12T20:29:48Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_FixedStatement.cs

+            VariableDeclaratorSyntax declarator = fixedInitializer.Syntax.FirstAncestorOrSelf<VariableDeclaratorSyntax>();
+            Debug.Assert(declarator != null);
+
+            pinnedTemp = factory.SynthesizedLocal(


// pinned ref int pinnedTemp #Closed

jcouv · 2018-02-12T20:30:29Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_FixedStatement.cs

+                getPinnableCall,
+                isRef: true);
+
+            // &pinnedTemp;


No semi-colon #Resolved

Nit: doesn't need a semi-colon: // &pinnedTemp

In reply to: 167677135 [](ancestors = 167677135)

jcouv

Done with review pass (iteration 8)

agocke · 2018-02-12T21:13:56Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

            SyntaxNode initializerSyntax = initializerOpt.Syntax;

-            if (ReferenceEquals(initializerType, null))
+            if ((object)initializerType == null)


Maybe is null now? #WontFix

== null is used throughout the file. I think we might want to fix that globally, but one change would just look odd.

In reply to: 167688236 [](ancestors = 167688236)

agocke · 2018-02-12T22:30:45Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+        {
+            if (initializer.Type.SpecialType == SpecialType.System_Void)
+            {
+                return null;


I think this method should be suffixed with "Opt" if it can return null. #Resolved

agocke · 2018-02-12T22:35:40Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                            ((object)getPinnableMethod == null || !extensisbleFixedEnabled || getPinnableMethod.ContainingType.SpecialType != SpecialType.System_String))
+                        {
+                            elementType = this.GetSpecialType(SpecialType.System_Char, diagnostics, initializerSyntax);
+                            Debug.Assert(!elementType.IsManagedType);


I don't understand what this assert is supposed to protect against. If the user code is wrong, why would we assert instead of provide an error? #Resolved

useless assert. it will always pass. removed.

In reply to: 167708843 [](ancestors = 167708843)

agocke · 2018-02-12T22:39:32Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+
+                        Error(diagnostics, ErrorCode.ERR_ExprCannotBeFixed, initializerSyntax);
                    }
+                    finally


This finally seems overly complicated. Couldn't we just stick the Error call in an else and always call this at the end of the if/else tree? That would remove all the break statements as well. #Resolved

agocke · 2018-02-12T22:45:13Z

src/Compilers/CSharp/Test/Semantic/Semantics/SemanticErrorTests.cs

-                Diagnostic(ErrorCode.ERR_FixedNotNeeded, "&i"),
-                // (14,26): error CS0213: You cannot use the fixed statement to take the address of an already fixed expression
+                Diagnostic(ErrorCode.ERR_FixedNotNeeded, "&i").WithLocation(7, 23),
+                // (14,26): error CS9365: The given expression cannot be used in a fixed statement


This error message seems worse (less specific) than the previous one. Why did it change? #ByDesign

The old message was actually incorrect. "already fixed" would apply if & was used. Here we do not use & . It might actually work if there is an extension method of right shape., but most likely it is just a typo.

I do not think the new message is worse than the old one.

In reply to: 167710934 [](ancestors = 167710934)

agocke · 2018-02-12T22:45:43Z

src/Compilers/CSharp/Test/Semantic/Semantics/SemanticErrorTests.cs

 ";
            CreateStandardCompilation(text, options: TestOptions.UnsafeReleaseDll).VerifyDiagnostics(
-                // (20,23): error CS0254: The right hand side of a fixed statement assignment may not be a cast expression
+                // (20,23): error CS9365: The given expression cannot be used in a fixed statement


Same here. #ByDesign

I think the old message was pretty random here. It only looks like it is more appropriate. The expression that we try to fix makes no sense in many ways.
I think this is the case where user of fixed should read documentation or check web for examples. It is not a lot to ask when unsafe features are involved.

In reply to: 167711048 [](ancestors = 167711048)

jcouv · 2018-02-20T22:50:33Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                    }
+                    else
+                    {
+                        // if feature was enabled but somethng went wrong with the method, report that


~~This comment seems out-of-date~~
Never mind #Closed

jcouv · 2018-02-20T22:51:12Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                    }
+                    else
+                    {
+                        // if feature was enabled but somethng went wrong with the method, report that


Typo: something #Resolved

jcouv · 2018-02-20T22:52:45Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                    }
+                    else
+                    {
+                        // if feature was enabled but somethng went wrong with the method, report that


I think we should report that the method was wrong regardless of the feature being enabled.
The errors should provide you a path (instructions) to enabling the feature and putting an appropriate method in place. #Resolved

I disagree on that.
The extra errors are primarily for those who develop the method, and they should have the feature enabled in the first place.
For others it would be just an annoyance - "please bump up the version to enable the feature, that you still cannot use and will only get more errors since the method that you very likely do not own or can change is broken"

I think if we have both cases - 1) feature is not enabled and 2) feature is unusable, we should not give extra diagnostics since that would not be actionable.

In reply to: 169487046 [](ancestors = 169487046)

Initially I did not consider that the diagnostics that we could not find a helper method is interesting at all. It was very noisy.
We would get those errors anywhere we try to pin something that we cannot pin (since nearly everything now can be pinned via an extension method, if such available).

Then I figured that there is one situation when you want those errors - when you are designing the actual helper, so I added some heuristics to detect that.
My criteria are - the feature is enabled and the method is "almost there, but not quite right".
Then I believe that someone is developing the helper and would want to see the extra diagnostics.

In reply to: 169494680 [](ancestors = 169494680,169487046)

OK. That works

jcouv · 2018-02-20T22:53:36Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                    }
+
+                    // check for a special ref-returning method
+                    var additionalDiagnostics = DiagnosticBag.GetInstance();


What was the motivation for removing the try/catch for freeing this instance? #Resolved

Andy's comment on this PR. I was undecided which way is better, so if someone also thinks that try/finally makes this more complex than needed, I've removed it

In reply to: 169487211 [](ancestors = 169487211)

jcouv

Done with review pass (iteration 9)

jcouv

LGTM

VSadov · 2018-02-21T03:34:55Z

@dotnet/roslyn-compiler - PING. One more review is needed.

agocke

LGTM

VSadov · 2018-02-22T00:11:00Z

Thanks!!

VSadov requested a review from a team as a code owner January 26, 2018 06:51

VSadov changed the title ~~Custom fixed~~ [WIP] Custom fixed Feb 8, 2018

VSadov added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Feb 8, 2018

VSadov force-pushed the patternFixed branch from fdd8253 to d04e037 Compare February 8, 2018 21:53

VSadov requested review from a team as code owners February 8, 2018 21:53

VSadov added 6 commits February 8, 2018 13:57

cleaned up fixed error reporting

0c41f1d

simple cases work

7cb61a2

Support structs and generics

3ff7674

more tests

2b88c1d

resources

3097ec9

not lifting to nullable

9ab10c2

VSadov force-pushed the patternFixed branch from d04e037 to 1b0a263 Compare February 8, 2018 21:58

VSadov changed the title ~~[WIP] Custom fixed~~ Extensible Fixed Statement Feb 8, 2018

VSadov removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Feb 8, 2018

Extra check for ref extensions. Various diagnostics and tests

f2a1db6

VSadov force-pushed the patternFixed branch from 1b0a263 to f2a1db6 Compare February 9, 2018 00:38

moved method name to a constant

d810de6

jcouv reviewed Feb 12, 2018

View reviewed changes

agocke reviewed Feb 12, 2018

View reviewed changes

PR feedback

276da81

jcouv reviewed Feb 20, 2018

View reviewed changes

More PR feedback

528b171

jcouv approved these changes Feb 21, 2018

View reviewed changes

VSadov added the Area-Compilers label Feb 21, 2018

agocke approved these changes Feb 21, 2018

View reviewed changes

VSadov merged commit 945c2c9 into dotnet:features/custom-fixed Feb 22, 2018

jcouv mentioned this pull request Mar 19, 2018

Test plan for "custom fixed" #25601

Open

36 tasks

briancylui mentioned this pull request Aug 3, 2018

Port C# key hardware intrinsics APIs for SSE from SIMD native algorithms dotnet/machinelearning#562

Merged


		// Note: you will need to re-generate compiler code after adding warnings (build\scripts\generate-compiler-code.cmd)

		ERR_ExprCannotBeFixed = 9365,

Extensible Fixed Statement #24469

Extensible Fixed Statement #24469

Uh oh!

Conversation

VSadov commented Jan 26, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VSadov commented Feb 9, 2018

Uh oh!

jcouv Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2018 • edited by VSadov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

VSadov commented Jan 26, 2018 •

edited by jcouv

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited by VSadov

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited by VSadov

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited

Loading

jcouv Feb 12, 2018 •

edited by VSadov

Loading

agocke Feb 12, 2018 •

edited by VSadov

Loading

agocke Feb 12, 2018 •

edited by VSadov

Loading